Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

backend start script: check ulimit and increase soft limit if possible #1234

Merged
merged 5 commits into from
Jul 18, 2024

Conversation

jstucke
Copy link
Collaborator

@jstucke jstucke commented Jul 3, 2024

No description provided.

@jstucke jstucke requested a review from dorpvom July 3, 2024 07:01
@jstucke jstucke self-assigned this Jul 3, 2024
@maringuu
Copy link
Collaborator

maringuu commented Jul 8, 2024

I think this code would be a good place to document why we need to increase the limit.

@jstucke
Copy link
Collaborator Author

jstucke commented Jul 15, 2024

I think this code would be a good place to document why we need to increase the limit.

I added a docstring to the _check_ulimit function

Comment on lines 118 to 124
"""
Each process has a hard limit and a soft limit for the maximum number of files opened at the same time. Since
FACT makes extensive use of multiprocessing features, it uses up a lot of those file descriptors and if we run
out, this raises an OSError. To mitigate this, we try to increase the soft limit and print a warning if the
hard limit is low. With the default configuration, FACT uses about 560 file descriptors (and potentially many
more if you crank up the worker counts).
"""
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can specify which multiprocessing features use file descriptors?
While I know that its queues, I would not expect the average reader to think of queues when thinking about multiprocessing.

Also, can you document the calculation that lead to 560 descriptors by default?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I extended the docstring quite a bit and added information on how the FDs are distributed

@maringuu maringuu self-requested a review July 18, 2024 09:24
@maringuu maringuu merged commit e6f63bd into master Jul 18, 2024
11 checks passed
@maringuu maringuu deleted the check-ulimit branch July 18, 2024 09:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants